Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

refactor cache manager for yurthub #265

Merged
merged 1 commit into from
Apr 22, 2021
Merged

refactor cache manager for yurthub #265

merged 1 commit into from
Apr 22, 2021

Conversation

rambohe-ch
Copy link
Member

Ⅰ. Describe what this PR does

Backgound:

  1. list requests from edge node have three types:
  • type1: only get specified object by using fieldselector=metadata.name=xxx, the request path like: https://{ip:port}/api/v1/namespaces/default/pods?fieldselector=metadata.name=xxx
  • type2: get a kind of objects by using fieldselector=spec.nodeName=xxx or labelselector=foo=bar, the request path like: https://{ip:port}/api/v1/pods?fieldselector=spec.nodeName=xxx
  • type3: get all pods, the request path like: https://{ip:port}/api/v1/pods
  1. cache manager handles list request in yurt-hub:

problem:

  • response for type3 list will not update all caches on local disk, so some deleted objects will remain in the cache.
  • if a client on edge node has both type2 and type3 list request, a error will occur for type2 response.

solution:

  1. replace local cache response instead of one by one cache for type2 and type3 list request
  2. fix cache conflict for type2 and type3 list request, a client can only support one type list request for one resource in type2 and type3。and at present, kubelet/kube-proxy/flannel/coredns have only one type list request for one resource in type2 and type3.

Ⅱ. Does this pull request fix one issue?

Ⅲ. List the added test cases (unit test/integration test) if any, please explain if no tests are needed.

Ⅳ. Describe how to verify it

make test
make all
and make sure yurthub cache manager can work

Ⅴ. Special notes for reviews

@rambohe-ch rambohe-ch force-pushed the refactor-cache-manager branch 2 times, most recently from 7c18345 to 2b60ff7 Compare April 20, 2021 07:14
Fei-Guo
Fei-Guo reviewed Apr 20, 2021
View reviewed changes
pkg/yurthub/storage/disk/storage.go
for key, data := range contents {
err := ds.create(key, data)
if err != nil {
klog.Errorf("failed to create %s in replace, %v", key, err)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If error happens, do we have retry mechanism? If not, do we have mechanism to do consistency check?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have not seriously thought about how to guarantee that object can be cached successfully. so i'd like to keep this subject as a TODO item. and we can solve it in the soon future.

pkg/yurthub/cachemanager/cache_manager.go Outdated
if info.Verb == "list" && info.Name == "" {
key, _ := util.KeyFunc(comp, info.Resource, info.Namespace, info.Name)
if _, ok := cm.listReqCollector[key]; !ok {
for k := range cm.listReqCollector {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you explain the purpose of having listReqCollector?

Also, can you give an example that "len(k) > len (key) " and an example for "len(k) < len(key)"?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. because func queryListObject() will get all pods for both requests instead of getting pods by request selector. so cache manager can not support same path list requests that has different selector. for example:
  • request1: http://{ip:port}/api/v1/default/pods?labelSelector=foo=bar
  • request2: http://{ip:port}/api/v1/default/pods?labelSelector=foo2=bar2
  1. because func queryListObject() will get all pods for both requests instead of getting pods by request selector. so cache manager can not support getting same resource list requests that has different path. for example:
  • request1: http://{ip/port}/api/v1/pods?fieldSelector=spec.nodeName=foo
  • request2: http://{ip/port}/api/v1/default/pods?fieldSelector=spec.nodeName=foo

in order to solve the above two problems, we need listReqCollector to keep list request info, and based on listReqCollector to verify list requests can cache or not.

Also, can you give an example that "len(k) > len (key) " and an example for "len(k) < len(key)"?
the second above example.

@rambohe-ch rambohe-ch force-pushed the refactor-cache-manager branch from 2b60ff7 to 8cb6608 Compare April 21, 2021 07:06
@rambohe-ch
refactor cache manager for yurthub
5f4fad2 
1. replace local cache response for list request
2. fix cache conflict for labelselector/fieldselector list and full list.
@rambohe-ch rambohe-ch force-pushed the refactor-cache-manager branch from 8cb6608 to 5f4fad2 Compare April 22, 2021 08:55
@Fei-Guo Fei-Guo merged commit b02fb17 into openyurtio:master Apr 22, 2021
@rambohe-ch rambohe-ch deleted the refactor-cache-manager branch May 27, 2021 06:26
MrGirl pushed a commit to MrGirl/openyurt that referenced this pull request Mar 29, 2022
@rambohe-ch
refactor cache manager for yurthub (openyurtio#265)
260888e 
1. replace local cache response for list request
2. fix cache conflict for labelselector/fieldselector list and full list.
@Congrool Congrool mentioned this pull request Jun 30, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Fei-Guo Fei-Guo Fei-Guo approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@rambohe-ch @Fei-Guo